Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allow for padding free plugin to be used without response template #430

Merged
merged 4 commits into from
Jan 9, 2025
Merged

feat: allow for padding free plugin to be used without response template #430

merged 4 commits into from
Jan 9, 2025

Conversation

dushyantbehl
Copy link
Contributor

@dushyantbehl dushyantbehl commented Jan 2, 2025 •
edited
Loading

Description of the change

Closes #426

Add support for using padding free plugin without response template.

Related issue number

Fixes #427

How to verify the PR

Was the PR tested

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 2, 2025

Thanks for making a pull request! 😃
One of the maintainers will review and advise on the next steps.

@github-actions github-actions bot added the feat label Jan 2, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 2, 2025

Thanks for making a pull request! 😃
One of the maintainers will review and advise on the next steps.

@fabianlim
Copy link
Collaborator

This PR is a duplicate of https://github.com/foundation-model-stack/fms-hf-tuning/pull/426/files. If this one supercedes, then should we close the other one

fabianlim
fabianlim previously approved these changes Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@fabianlim fabianlim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to not make is_padding_free so explicit as I have suggested in the duplicate PR. but otherwise it is ok so I approve it. But good to have @Ssukriti @anhuong @willmj @asm582 to have some eyes before merging

@dushyantbehl
Copy link
Contributor Author

dushyantbehl commented Jan 2, 2025 •
edited
Loading

This PR is a duplicate of https://github.com/foundation-model-stack/fms-hf-tuning/pull/426/files. If this one supercedes, then should we close the other one

Yes, this closes #426 as mentioned in the top level comment.

@dushyantbehl dushyantbehl mentioned this pull request Jan 2, 2025
Closed
2 tasks
fabianlim
fabianlim reviewed Jan 2, 2025
View reviewed changes
tuning/sft_trainer.py Outdated Show resolved Hide resolved
@HarikrishnanBalagopal
Copy link
Contributor

HarikrishnanBalagopal commented Jan 6, 2025 •
edited
Loading

Have done some end to end testing for training the ibm-granite/granite-3.0-8b-base model using padding free extended pre training on a jsonl dataset with 1 node 8 GPUs. Tested commit details:

* commit 8674d82e6490e6d1e4f1ed9efc16c4e5c8794073 (HEAD -> dushyantbehl-feat-padding-free, origin/dushyantbehl-feat-padding-free)
| Author: Dushyant Behl <[email protected]>
| Date:   Thu Jan 2 15:16:12 2025 +0530
| 
|     add data collator for padding free plugin scenario to be used for extended pretraining
|     
|     Signed-off-by: Dushyant Behl <[email protected]>
|

We can merge this PR.

@dushyantbehl
Copy link
Contributor Author

Added a new commit with

  1. New unit tests.
  2. Change requested by @fabianlim

And as e2e testing is already done by @HarikrishnanBalagopal

The PR is ready to be merged now. Please mark @HarikrishnanBalagopal as co-author while merging.

cc @fabianlim @ashokponkumar

@fabianlim
Copy link
Collaborator

my changes are addressed.. pls address @kmehant 's changes

HarikrishnanBalagopal and others added 3 commits January 9, 2025 13:46
@HarikrishnanBalagopal @kmehant
fix: allow for padding free + pretraining
e1b1ff0 
Signed-off-by: Harikrishnan Balagopal <[email protected]>
@dushyantbehl @kmehant
add data collator for padding free plugin scenario to be used for ext…
9612ac4 
…ended pretraining

Signed-off-by: Dushyant Behl <[email protected]>
@kmehant
fix: update value error
108180d 
Signed-off-by: Mehant Kammakomati <[email protected]>
@kmehant kmehant force-pushed the feat/padding-free branch from 685621e to 108180d Compare January 9, 2025 08:16
@kmehant
fix: delete images only when exists
eb5222a 
Signed-off-by: Mehant Kammakomati <[email protected]>
kmehant
kmehant approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@kmehant kmehant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requests from all the reviewers are addressed.

@kmehant kmehant merged commit 53a9d18 into foundation-model-stack:main Jan 9, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianlim fabianlim fabianlim left review comments

@ashokponkumar ashokponkumar ashokponkumar left review comments

@kmehant kmehant kmehant approved these changes

@anhuong anhuong Awaiting requested review from anhuong anhuong is a code owner

@Ssukriti Ssukriti Awaiting requested review from Ssukriti Ssukriti is a code owner

@aluu317 aluu317 Awaiting requested review from aluu317 aluu317 is a code owner

Assignees
No one assigned
Labels
feat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

padding_free with extended pretraining causes a crash
5 participants
@dushyantbehl @fabianlim @HarikrishnanBalagopal @ashokponkumar @kmehant